Skip to content

[Wordy] Clean up Approaches#4198

Merged
BethanyG merged 9 commits into
exercism:mainfrom
Yrahcaz7:wordy-approaches-cleanup
May 22, 2026
Merged

[Wordy] Clean up Approaches#4198
BethanyG merged 9 commits into
exercism:mainfrom
Yrahcaz7:wordy-approaches-cleanup

Conversation

@Yrahcaz7
Copy link
Copy Markdown
Contributor

As discussed in #4055 and #4197.

also some formatting fixes
@github-actions

This comment was marked as resolved.

@github-actions github-actions Bot closed this May 21, 2026
@BethanyG BethanyG reopened this May 21, 2026
@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Yrahcaz7 commented May 21, 2026

Briefly browsing through the rest of the approaches, it seems like my solution uses yet another approach. Maybe I'll add an approach doc for it after I finish auditing all of the others.

Comment thread exercises/practice/wordy/.approaches/introduction.md
@BethanyG
Copy link
Copy Markdown
Member

Thought I'd start with a read-through. Looking good so far. PSA: Don't forget to add yourself as a contributor for any approach that you fix/edit. 😄

I'll come back and do another once you're ready.

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Yrahcaz7 commented May 21, 2026

That will probably be sometime tomorrow, considering that it's getting pretty late in my timezone.

@BethanyG
Copy link
Copy Markdown
Member

Briefly browsing through the rest of the approaches, it seems like my solution uses yet another approach. Maybe I'll add an approach doc for it after I finish auditing all of the others.

awesome sauce! I love it. The more approaches, the better. I find that a wide variety teaches a TON of things. It can become an obsession. 😉

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Audit part 2

introduction.md

Line 210

The concept link ([concept: unpacking and multiple assignment](/tracks/python/concepts/unpacking-and-multiple-assignment)) does not have the hover modal, even though string-list-and-dict-methods/content.md has the same exact link and does have the hover modal.

I can't find any docs on this, but searching through the repo, I find links like the following: [concept:python/unpacking-and-multiple-assignment](), and these seem to work. The links in cater-waiter/.docs/hints.md seem to be similarly broken, should I open a PR for these?

import-callables-from-operator/content.md

  • Line 43 has a random aside that does not fit with the sentence structure: (floordiv is aliased to "div"). I think this would be better expanded into its own sentence.

  • Line 44 is confusing, the methods are always callables, using () just calls them, it doesn't somehow make them callables. Maybe "used as callables" was meant instead of "made callable"?

  • Some terms are missing backticks around them.

Both

  • Some code blocks have mixed double quotes and single quotes.

also more formatting fixes
@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Audit part 3

introduction.md and regex-with-operator-module/content.md

  • Some code blocks have mixed double quotes and single quotes.

regex-with-operator-module/content.md

  • The code block has some strangely wrapped comments and some comments that do not end with punctuation.

  • Line 88: This has the same issue as the previous one with the "then made callable using ()".

  • Line 88: "result" is not surrounded with backticks.

  • Some minor grammar issues.

Question: Some terms are referred to in multiple ways, such as "while-loop" vs "while loop" and "list-comprehension" vs "list comprehension". Which version should be used?

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Audit part 4

lambdas-in-a-dictionary/content.md

  • Line 38 (line 297 in introduction.md): "OPERATIONS" is not surrounded by backticks.

  • The variant here has trailing underscores in the function names, but leading underscores would make more sense, as there are no Python keywords with those names. PEP 8 says "single_trailing_underscore_: used by convention to avoid conflicts with Python keyword"

  • Line 89: the hyphen should be an em-dash.

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Audit part 5

recursion/content.md and introduction.md

  • This approach has an unnecessary else after the return for the base case in calculate(). Pylint reports "R1705 no-else-return" and suggests removing it, and I agree 😆.

recursion/content.md

  • These code blocks have some strangely wrapped comments and some comments that do not end with punctuation.

  • In some places, the operator module is just referred to as operator, which is confusing.

  • Some terms are missing backticks around them.

  • The last code block has mixed double quotes and single quotes.

  • Lines 242 to 248: Some list items are missing punctuation at the end.

  • Variation 2 doesn't seem to work? It uses match as a variable, but it is a keyword. And even if I fix that, isn't it processing the operations in the wrong order?

@BethanyG
Copy link
Copy Markdown
Member

Variation 2 doesn't seem to work? It uses match as a variable, but it is a keyword. And even if I fix that, isn't it processing the operations in the wrong order?

This one? When I put it into the online editor, it was complaining about an indentation error. I went back over it, and the version below now passes all the tests.

import re
from operator import add, mul, sub
from operator import floordiv as div

DIGITS = re.compile(r"-?\d+")
OPERATORS = (
    (mul, re.compile(r"(?P<x>.*) multiplied by (?P<y>.*)")),
    (div, re.compile(r"(?P<x>.*) divided by (?P<y>.*)")),
    (add, re.compile(r"(?P<x>.*) plus (?P<y>.*)")),
    (sub, re.compile(r"(?P<x>.*) minus (?P<y>.*)")),
    )

def answer(question):
    if not question.startswith( "What is") or "cubed" in question:
        raise ValueError("unknown operation")
    
    question = question.removeprefix( "What is").removesuffix("?").strip()

    if not question:
        raise ValueError("syntax error")
    
    return calculate(question)

def calculate(question):
    if DIGITS.fullmatch(question):
        return int(question)
        
    for operation, pattern in OPERATORS:
        if match := pattern.match(question): #<-- The walrus operator assigns the new re match to `match` each loop.
            return operation(calculate(match['x']), calculate(match['y']))#<-- the loop is paused here to make the two recursive calls.
    raise ValueError("syntax error")

Careful of that match "variable". match is indeed a keyword when you use it with a match-case. BUT it is a "soft keyword" - which means it can still be used as a name in places where it won't get confused with match-case (a.k.a. where it isn't followed by indents and case). Soft keywords are sorta terrible, but I get why it happened. I can't find the docs at the moment, but the discussion around match-case had folx concerned about making them hard keywords, since there was 20+ years of code that potentially used those as identifiers, so would break if they became fully reserved.

Also note that this uses a walrus operator, which means the value of match is changing with every call to calculate().

If it is calculating in the wrong order, at least a few of the tests should have failed. But this is also a very complex solution, since it has two recursive branches in a loop. IIRC, when I was going over this, I had to put it into python tutor and walk it to make sure I really understood the recursive calls in the loop.

This actually came from Isaac. You can see his solution variations here. The recursion in a loop variants are the first three, I think.

@BethanyG
Copy link
Copy Markdown
Member

This approach has an unnecessary else after the return for the base case in calculate(). Pylint reports "R1705 no-else-return" and suggests removing it, and I agree 😆.

Yup. Definitely cleaner that way (I should listen to PyLint more often). I need to update my published solution! 😆

from operator import add, mul, sub
from operator import floordiv as div

# Define a lookup table for mathematical operations
OPERATIONS = {"plus": add, "minus": sub, "multiplied": mul, "divided": div}


def answer(question):
    # Call clean() and feed it to calculate()
    return calculate(clean(question))

def clean(question):
    # It's not a question unless it starts with 'What is'.
    if not question.startswith("What is") or "cubed" in question:
        raise ValueError("unknown operation")

    # Remove the unnecessary parts of the question and
    # parse the cleaned question into a list of items to process.
    # The wrapping () invoke implicit concatenation for the chained functions
    return (question.removeprefix("What is")
            .removesuffix("?")
            .replace("by", "")
            .strip()).split() # <-- this split() turns the string into a list.

# Recursively calculate the first piece of the equation, calling 
# calculate() on the product + the remainder.
# Return the solution when len(equation) is one.
def calculate(equation):
    if len(equation) == 1:
        return int(equation[0])

    try:
        # Unpack the equation into first int, operator, and second int.
        # Stuff the remainder into *rest
        x_value, operation, y_value, *rest = equation

        # Redefine the equation list as the product of the first three
        # variables concatenated with the unpacked remainder.
        equation = [OPERATIONS[operation](int(x_value),
                                          int(y_value)), *rest]
    except:
        raise ValueError("syntax error")

    # Call calculate() with the redefined/partially reduced equation.
    return calculate(equation)

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Yrahcaz7 commented May 21, 2026

Careful of that match "variable". match is indeed a keyword when you use it with a match-case. BUT it is a "soft keyword" - which means it can still be used as a name in places where it won't get confused with match-case (a.k.a. where it isn't followed by indents and case).

I see. I didn't know about soft keywords.

If it is calculating in the wrong order, at least a few of the tests should have failed.

Turns out there actually isn't a test for the case that I'm thinking of! There are no questions in the test suite with at least three operations/four numbers in them.

This pythontutor link should show the case I'm talking about.

You can see that since it parses the expression in the wrong order, it returns 9 instead of -31.

@BethanyG
Copy link
Copy Markdown
Member

Turns out there actually isn't a test for the case that I'm thinking of! There are no questions in the test suite with at least three operations/four numbers in them.

This pythontutor link should show the case I'm talking about.

You can see that since it parses the expression in the wrong order, it returns 9 instead of -31.

This is an excellent catch!!. Methinks we should add some additional track-specific test cases for this exercise. NICE WORK!!!

Here are my current thoughts:

  1. We get all the small changes and corrections done for this.
  2. We change the explanation for Variation 2 to point at a failure to generalize, due to items being processed in the incorrect order.
  3. We (attempt) to correct the code so that it does process in the correct order (not sure we can do that with the looped recursion, but we can try) and add it to the approach.
  4. We test the other approaches for the same issue (since we have a hole in test coverage that is significant)
  5. We add several additional test cases in the additional test case JSON, to cover this hole
  6. If the example for this exercise also fails the new test cases, we might want to start a discussion on the forum about missing test cases in the canonical data. I'd wait to do this to make absolutely sure that a large number of solutions across programming languages would fail the new cases, otherwise you'll get denied.

Make sense? 😄

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

I think so! Number 5 would be in a separate PR, right?

@BethanyG
Copy link
Copy Markdown
Member

BethanyG commented May 21, 2026

I think so! Number 5 would be in a separate PR, right?

Yuppers. A tad more involved than it looks, since we need to also regenerate the test file, may or may not need to adjust the test template (likely not), and do some re-tests on the example.

And 6 would be an entirely different repo, Problem Specs with likely a long discussion among maintainers on the forum first. For that, you'll want some illustrative examples from both Python and other languages.

@BethanyG
Copy link
Copy Markdown
Member

Let's get the major changes here merged, and then I can walk you through the test generation/template/additional tests thing. If that's OK?

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Sounds good!

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Yrahcaz7 commented May 21, 2026

The last two approaches basically just had the same kind of errors that were present in the previous ones. Though the very last one was also too compact, so I expanded it so it would be readable.

@Yrahcaz7
Copy link
Copy Markdown
Contributor Author

Yrahcaz7 commented May 21, 2026

@BethanyG, that's numbers 1 and 2 done; now it should be time for 3 and 4. I'm not quite sure how to go about doing these, however.

Especially number 3... That variant is super complicated.

@BethanyG
Copy link
Copy Markdown
Member

@BethanyG, that's numbers 1 and 2 done; now it should be time for 3 and 4. I'm not quite sure how to go about doing these, however.

Especially number 3... That variant is super complicated.

Oh! I am so sorry! Somehow, past a certain point, comments don't seem to be triggering new notifications for me. I was head's down on fixing classes (spoiler: it might go the way of that other one), and didn't get pinged.

Give me a few, and let me see if I can take a crack. 😄

@BethanyG
Copy link
Copy Markdown
Member

As for 4, are you testing locally with PyTest, or using the website? If you are testing locally, we can do a special hand-edit to the test file and you can then use that for the other approaches.

If not...well, time to learn how to set up a local testing env with PyTest. 😉

Ping me on the forum or Discord, and I can walk you through. I don't trust GH at the moment. 😡

@BethanyG
Copy link
Copy Markdown
Member

Give me a few, and let me see if I can take a crack. 😄

Today, children, we are going to learn about positive lookaheads in regex 🤦🏽‍♀️ . I mean, why not? I don't really need those neurons, do I?

Copy link
Copy Markdown
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two comments. The first is due to the flakey rendering on the site, which sometimes doesn't find the reflink unless it's inside the notes block. Horrifying in the case of a pythontutor link, but worth it I think.

The second is just admiration. 😄

Comment thread exercises/practice/wordy/.approaches/recursion/content.md
@BethanyG BethanyG marked this pull request as ready for review May 22, 2026 01:52
Co-authored-by: BethanyG
<BethanyG@users.noreply.github.com>
@Yrahcaz7 Yrahcaz7 requested a review from BethanyG May 22, 2026 01:58
Copy link
Copy Markdown
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@BethanyG
Copy link
Copy Markdown
Member

Big, green button or a cooling-off period?

@BethanyG BethanyG merged commit 323ada8 into exercism:main May 22, 2026
7 checks passed
@Yrahcaz7 Yrahcaz7 deleted the wordy-approaches-cleanup branch May 22, 2026 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants